feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear #2646

bizob2828 · 2024-10-11T19:23:34Z

This PR is a lot so I apologize. This removes transaction from segment, introduces a context class for storing the active transaction and segment as well as helpers to create a new context for entering a new segment and transaction.

Note: You will see two instances where enterSegment passes in the transaction because the 3rd party promise intrumentation has its own context manager. Some follow up work will occur in #2657. This is targeting next as this is quite a radical change and we need to do more manual testing once all the work is done

codecov · 2024-10-11T19:57:18Z

Codecov Report

Attention: Patch coverage is 99.69072% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (next@5275a3c). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
lib/shim/shim.js	98.96%	2 Missing ⚠️
lib/instrumentation/when/index.js	96.77%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #2646   +/-   ##
=======================================
  Coverage        ?   97.25%           
=======================================
  Files           ?      291           
  Lines           ?    46251           
  Branches        ?        0           
=======================================
  Hits            ?    44981           
  Misses          ?     1270           
  Partials        ?        0

Flag	Coverage Δ
integration-tests-cjs-18.x	`74.14% <72.30%> (?)`
integration-tests-cjs-20.x	`74.15% <72.30%> (?)`
integration-tests-cjs-22.x	`74.19% <72.30%> (?)`
integration-tests-esm-18.x	`49.59% <28.85%> (?)`
integration-tests-esm-20.x	`49.60% <28.85%> (?)`
integration-tests-esm-22.x	`49.62% <28.85%> (?)`
unit-tests-18.x	`88.83% <80.80%> (?)`
unit-tests-20.x	`88.83% <80.80%> (?)`
unit-tests-22.x	`88.83% <80.80%> (?)`
versioned-tests-18.x	`79.07% <87.91%> (?)`
versioned-tests-20.x	`79.09% <87.91%> (?)`
versioned-tests-22.x	`79.09% <87.91%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

enterTransaction

jsumners-nr

As expected, most of the changes are signature related updates. The majority of my comments are around making sure our inline docs are correct. But I do have a couple of clarifying questions.

jsumners-nr · 2024-10-24T18:58:04Z

lib/agent.js

@@ -843,7 +842,7 @@ Agent.prototype.getLinkingMetadata = function getLinkingMetadata() {
  }

  if (config.distributed_tracing.enabled && segment) {


This looks like the only usage of segment in this function, and it was previously used only to get the transaction. I think we can remove const segment = this.tracer.getSegment().

the line right below it is using segment to get the span id

jsumners-nr · 2024-10-24T19:00:25Z

lib/db/parsed-statement.js

@@ -21,10 +21,9 @@ function ParsedStatement(type, operation, collection, raw) {
  }
 }

-ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope) {
+ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope, transaction) {


Do you think we will eventually remove segment from the parameters and then retrieve it from the transaction in the function body?

I don't understand. Recorders are registered at the start of recording of a segment. In tracer they are added to transaction and binding the appropriate segment:

if (recorder) { transaction.addRecorder(recorder.bind(null, segment)) }

Then once a transaction end it iterates over all the recorders and calls the function with the scope and the transaction

Transaction.prototype.record = function record() { const name = this.name for (let i = 0, l = this._recorders.length; i < l; ++i) { this._recorders[i](name, this) } }

Doesn't this work make transaction the entry point of the API? As in, wouldn't we attempt to retrieve a segment from the transaction?

lib/instrumentation/aws-sdk/v3/bedrock.js

jsumners-nr · 2024-10-24T19:03:04Z

lib/instrumentation/aws-sdk/v3/bedrock.js

I suspect all of the functions that are getting a new transaction parameter can also drop the segment parameter.

we need the segment because it's getting ended

lib/instrumentation/kafkajs/consumer.js

test/integration/transaction/tracer.tap.js

test/unit/shim/shim.test.js

test/unit/shim/webframework-shim.test.js

test/unit/transaction/tracer.test.js

bizob2828

addressed all comments and responded to questions in here

bizob2828 · 2024-10-24T18:55:33Z

test/versioned-external/external-repos.js

@@ -15,7 +15,7 @@ const repos = [
  {
    name: 'apollo-server',
    repository: 'https://github.com/newrelic/newrelic-node-apollo-server-plugin.git',
-    branch: 'main',
+    branch: 'remove-transaction-from-segment',


so I had to update this repo too. we're going to have to keep track of this when we release

bizob2828 · 2024-10-30T15:27:22Z

lib/agent.js

@@ -843,7 +842,7 @@ Agent.prototype.getLinkingMetadata = function getLinkingMetadata() {
  }

  if (config.distributed_tracing.enabled && segment) {


the line right below it is using segment to get the span id

bizob2828 · 2024-10-30T15:31:08Z

lib/db/parsed-statement.js

@@ -21,10 +21,9 @@ function ParsedStatement(type, operation, collection, raw) {
  }
 }

-ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope) {
+ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope, transaction) {


I don't understand. Recorders are registered at the start of recording of a segment. In tracer they are added to transaction and binding the appropriate segment:

if (recorder) { transaction.addRecorder(recorder.bind(null, segment)) }

Then once a transaction end it iterates over all the recorders and calls the function with the scope and the transaction

Transaction.prototype.record = function record() { const name = this.name for (let i = 0, l = this._recorders.length; i < l; ++i) { this._recorders[i](name, this) } }

bizob2828 · 2024-10-30T15:34:30Z

lib/instrumentation/aws-sdk/v3/bedrock.js

we need the segment because it's getting ended

bizob2828 · 2024-10-30T17:22:34Z

lib/transaction/index.js

@@ -22,6 +22,7 @@ const Logs = require('./logs')
 const DT_ACCEPT_PAYLOAD_EXCEPTION_METRIC = 'DistributedTrace/AcceptPayload/Exception'
 const DT_ACCEPT_PAYLOAD_PARSE_EXCEPTION_METRIC = 'DistributedTrace/AcceptPayload/ParseException'
 const REQUEST_PARAMS_PATH = 'request.parameters.'
+const TraceStacks = require('../util/trace-stacks')


i'm going to remove this trace stacks idea. it's a debug thing that seems useless

lib/transaction/tracer/index.js

bizob2828 · 2024-10-30T17:57:48Z

lib/transaction/tracer/index.js

  wrapped[symbols.original] = getOriginal(handler)
-  wrapped[symbols.segment] = active
+  // wrapped[symbols.segment] = segment


I'll restore this but not sure if we need it

lib/util/segments.js

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 · 2024-10-31T15:01:31Z

Closes #2658

… and enterTransaction to make context propagation more clear (#2646)

… and enterTransaction to make context propagation more clear (newrelic#2646)

… and enterTransaction to make context propagation more clear (#2646)

jlowcs · 2025-01-24T15:20:39Z

This change introduced a breaking change that broke @newrelic/apollo-server-plugin

TypeError: Cannot read properties of undefined (reading 'measure')
    at createMetricPairs (/node_modules/@newrelic/apollo-server-plugin/lib/common.js:116:17)
    at recordOperationSegment (/node_modules/@newrelic/apollo-server-plugin/lib/common.js:111:3)
    at Transaction.record (/node_modules/newrelic/lib/transaction/index.js:680:23)
    at Transaction.end (/node_modules/newrelic/lib/transaction/index.js:212:10)
    at ServerResponse.instrumentedFinish (/node_modules/newrelic/lib/instrumentation/core/http.js:207:15)
    at <anonymous> (/node_modules/@opentelemetry/context-async-hooks/src/AbstractAsyncHooksContextManager.ts:75:49)
    at AsyncLocalStorage.run (node:internal/async_local_storage/async_hooks:91:14)
    at SentryContextManager.with (/node_modules/@opentelemetry/context-async-hooks/src/AsyncLocalStorageContextManager.ts:40:36)
    at SentryContextManager.with (/node_modules/@sentry/opentelemetry/src/contextManager.ts:71:24)
    at ServerResponse.contextWrapper (/node_modules/@opentelemetry/context-async-hooks/src/AbstractAsyncHooksContextManager.ts:75:26)
    at Object.onceWrapper (node:events:632:28)
    at ServerResponse.emit (node:events:530:35)
    at AsyncLocalStorage.run (node:internal/async_local_storage/async_hooks:91:14)
    at AsyncLocalContextManager.runInContext (/node_modules/newrelic/lib/context-manager/async-local-context-manager.js:58:38)
    at ServerResponse.wrapped [as emit] (/node_modules/newrelic/lib/transaction/tracer/index.js:250:37)
    at onFinish (node:_http_outgoing:1077:10)
    at callback (node:internal/streams/writable:766:21)
    at afterWrite (node:internal/streams/writable:710:5)
    at afterWriteTick (node:internal/streams/writable:696:10)
    at process.processTicksAndRejections (node:internal/process/task_queues:89:21)

I just filed an issue: #2903

bizob2828 force-pushed the remove-tx-from-segment branch 3 times, most recently from a876112 to 94f381a Compare October 16, 2024 16:05

bizob2828 force-pushed the remove-tx-from-segment branch from 94f381a to 3cd3159 Compare October 24, 2024 17:39

bizob2828 changed the base branch from main to next October 24, 2024 18:13

bizob2828 changed the title ~~Remove tx from segment~~ feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear Oct 24, 2024

bizob2828 marked this pull request as ready for review October 24, 2024 18:50

bizob2828 force-pushed the remove-tx-from-segment branch from cb93d08 to c2afb3f Compare October 24, 2024 18:51

feat: Updated agent to no longer store transaction segment

b7d207d

bizob2828 force-pushed the remove-tx-from-segment branch from c2afb3f to b7b4ef8 Compare October 24, 2024 18:52

bizob2828 requested a review from jsumners-nr October 24, 2024 18:53

feat: Added a Context class and methods to enterSegment and

17ea0db

enterTransaction

bizob2828 force-pushed the remove-tx-from-segment branch from b7b4ef8 to 17ea0db Compare October 24, 2024 19:19

jsumners-nr reviewed Oct 24, 2024

View reviewed changes

bizob2828 mentioned this pull request Oct 28, 2024

chore: Updated instrumented to get the transaction directly instead of from the active segment newrelic/csec-node-agent#261

Merged

chore: addressed PR feedback

a2f19da

bizob2828 commented Oct 30, 2024

View reviewed changes

bizob2828 requested a review from jsumners-nr October 30, 2024 19:13

jsumners-nr approved these changes Oct 30, 2024

View reviewed changes

bizob2828 merged commit d726132 into next Oct 31, 2024
48 checks passed

bizob2828 deleted the remove-tx-from-segment branch October 31, 2024 12:44

bizob2828 added a commit that referenced this pull request Oct 31, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

27d8484

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Oct 31, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

58aaf88

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Nov 7, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

3f806bd

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Nov 8, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

ad7628b

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Nov 13, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

ce7eb39

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Nov 19, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

61ff337

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Dec 4, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

f8777b1

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Dec 9, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

cf38ce6

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Dec 12, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

c6d59bd

… and enterTransaction to make context propagation more clear (#2646)

jsumners-nr pushed a commit to jsumners-nr/node-newrelic that referenced this pull request Dec 13, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

31a4209

… and enterTransaction to make context propagation more clear (newrelic#2646)

bizob2828 added a commit that referenced this pull request Dec 13, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

f9fcdf6

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Dec 16, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

da1bb46

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 mentioned this pull request Dec 18, 2024

fix: Updated shim.createSegment and shim.handleMqTracingHeaders to be backwards compatible with the changes to context manager #2844

Merged

bizob2828 added a commit that referenced this pull request Dec 19, 2024

feat: Removed transaction from segment. Introduced a new enterSegment…

8fa1a15

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 added a commit that referenced this pull request Jan 13, 2025

feat: Removed transaction from segment. Introduced a new enterSegment…

d84531a

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 mentioned this pull request Jan 13, 2025

chore: Merge staged otel work #2873

Merged

bizob2828 added a commit that referenced this pull request Jan 14, 2025

feat: Removed transaction from segment. Introduced a new enterSegment…

7078554

… and enterTransaction to make context propagation more clear (#2646)

bizob2828 mentioned this pull request Jan 14, 2025

chore: release v12.11.0 #2877

Merged

jlowcs mentioned this pull request Jan 24, 2025

[12.11.0] breaking change introduced resulting in failure in @newrelic/apollo-server-plugin #2903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear #2646

feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear #2646

bizob2828 commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

jsumners-nr left a comment

jsumners-nr Oct 24, 2024

bizob2828 Oct 30, 2024

jsumners-nr Oct 24, 2024

bizob2828 Oct 30, 2024

jsumners-nr Oct 30, 2024

jsumners-nr Oct 24, 2024

bizob2828 Oct 30, 2024

bizob2828 left a comment

bizob2828 Oct 24, 2024

bizob2828 Oct 30, 2024

bizob2828 Oct 30, 2024

bizob2828 Oct 30, 2024

bizob2828 Oct 30, 2024

bizob2828 Oct 30, 2024

bizob2828 commented Oct 31, 2024

jlowcs commented Jan 24, 2025 •

edited

Loading

		@@ -843,7 +842,7 @@ Agent.prototype.getLinkingMetadata = function getLinkingMetadata() {
		}

		if (config.distributed_tracing.enabled && segment) {

feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear #2646

feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear #2646

Conversation

bizob2828 commented Oct 11, 2024 • edited Loading

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

jsumners-nr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 commented Oct 31, 2024

jlowcs commented Jan 24, 2025 • edited Loading

bizob2828 commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

jlowcs commented Jan 24, 2025 •

edited

Loading